Allow insert_all on relation #38899
Allow insert_all on relation #38899
Conversation
|
How does this behave for has many through relation now? |
|
Doesn't work expected, need to handle that as an error as you did. |
kamipo
force-pushed
the
insert_all_on_relation
branch
from
05cb829
to
c947e35
Compare
|
@kamipo I see you're still working on this. Feel free to ping me once you're done, I can take a look as well. |
|
I've done. |
|
@kamipo IMHO |
|
Yes, currently it is not easy to check for through relation not on association relation. |
| class_eval <<~RUBY | ||
| def #{method}(attributes, **kwargs) | ||
| if @association.reflection.through_reflection? | ||
| raise ArgumentError, "Bulk insert is currently not supported for through relations" |
There was a problem hiding this comment.
@kamipo Can we change the error message to say Bulk insert or update since upsert perform updates? Also, through relations -> has_many :through association if that looks proper to you.
| raise ArgumentError, "Bulk insert is currently not supported for through relations" | |
| raise ArgumentError, "Bulk insert or update is currently not supported for has_many :through association" |
There was a problem hiding this comment.
Also, is ArgumentError correct here? We haven't implemented bulk insert for has_many through association. Shouldn't it be NotImplementedError?
There was a problem hiding this comment.
AFAIK NotImplementedError is not meant to be raised for missing app features, but for missing platform features. https://ruby-doc.org/core-2.5.0/NotImplementedError.html
|
@kamipo I can take a look if there is not any chance to expose this somehow to be accessible in |
|
Updated the error message at b69ff2ed39c79886ba4b606c2bb75db8fac323a4. @abhaynikam Is it fine for you? I agree that |
|
LGTM @kamipo 👍, Thanks. |
|
Actually we can check whether association relation or not by diff --git a/activerecord/lib/active_record/insert_all.rb b/activerecord/lib/active_record/insert_all.rb
index 3d6d1df18a..fbe7f4fc74 100644
--- a/activerecord/lib/active_record/insert_all.rb
+++ b/activerecord/lib/active_record/insert_all.rb
@@ -13,7 +13,10 @@ def initialize(model, inserts, on_duplicate:, returning: nil, unique_by: nil)
@model, @connection, @inserts, @keys = model, model.connection, inserts, inserts.first.keys.map(&:to_s)
@on_duplicate, @returning, @unique_by = on_duplicate, returning, unique_by
- if model.scope_attributes?
+ if scope = model.scope_attributes?
+ if scope.respond_to?(:proxy_association) && scope.proxy_association.reflection.through_reflection?
+ raise ArgumentError, "Bulk insert or upsert is currently not supported for has_many through association"
+ end
@scope_attributes = model.scope_attributes
@keys |= @scope_attributes.keys
end |
|
@kamipo yes, that's similar to that one I used before. That's hackish as well. I would like to take a look for potential refactoring to get this info easier in |
| @@ -108,7 +114,6 @@ def unique_by_columns | |||
| Array(unique_by&.columns) | |||
| end | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
All these newlines were intentional for method grouping.
There was a problem hiding this comment.
I see... will add newlines back.
|
I will add a bit more tests and a CHANGELOG entry before merging. |
|
Awesome, sounds great 🤘 |
This allows to work `author.books.insert_all` as expected. Co-authored-by: Josef Šimánek <josef.simanek@gmail.com>
kamipo
force-pushed
the
insert_all_on_relation
branch
from
b69ff2e
to
5cec7ce
Compare
|
Great job everyone! I'm putting on my list to investigate through relations support as well. |
|
👏 I'm excited for this!! |
Adds docs for rails#38899 cc @kamipo
This allows to work
author.books.insert_allas expected.